Apply effects to `otherwise` edge in dataflow analysis #142707

ashivaram23 · 2025-06-19T07:35:59Z

This allows ElaborateDrops to remove drops when a match wildcard arm covers multiple no-Drop enum variants. It modifies dataflow analysis to update the MaybeUninitializedPlaces and MaybeInitializedPlaces data for a block reached through an otherwise edge.

Fixes #142705.

rustbot · 2025-06-19T07:36:04Z

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

petrochenkov · 2025-06-19T15:04:01Z

r? compiler

workingjubilee · 2025-06-19T18:35:38Z

hm.
@bors2 try @rust-timer queue

rust-bors · 2025-06-19T18:35:42Z

⌛ Trying commit 5837955 with merge e3d7e41…

To cancel the try build, run the command @bors2 try cancel.

Update `MaybeUninitializedPlaces` and `MaybeInitializedPlaces` for `otherwise`    This allows `ElaborateDrops` to remove drops when a `match` wildcard arm covers multiple no-Drop enum variants. It modifies dataflow analysis to update the `MaybeUninitializedPlaces` and `MaybeInitializedPlaces` data for a block reached through an `otherwise` edge. This appears to fix #142705, but I don't know for sure if it's actually correct (are there cases where this would be wrong and break things?). I also haven't tested that it actually improves compile times, or machine code output outside of the examples in the issue.

rust-bors · 2025-06-19T21:05:46Z

☀️ Try build successful (CI)
Build commit: e3d7e41 (e3d7e41d7e8c513f55398f6e34eb2af7fb7df49f, parent: 8de4c7234dd9b97c9d76b58671343fdbbc9a433e)

rust-timer · 2025-06-20T03:38:26Z

Finished benchmarking commit (e3d7e41): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	1.1%	[0.6%, 1.5%]	2
Regressions ❌ (secondary)	0.6%	[0.4%, 0.8%]	4
Improvements ✅ (primary)	-0.5%	[-1.2%, -0.2%]	32
Improvements ✅ (secondary)	-0.7%	[-1.3%, -0.2%]	19
All ❌✅ (primary)	-0.4%	[-1.2%, 1.5%]	34

Max RSS (memory usage)

Results (primary -0.2%, secondary 2.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.5%	[2.3%, 4.7%]	3
Regressions ❌ (secondary)	2.8%	[2.2%, 3.5%]	2
Improvements ✅ (primary)	-2.4%	[-3.6%, -0.5%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-3.6%, 4.7%]	8

Cycles

Results (primary 0.9%, secondary -5.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.7%	[2.6%, 2.9%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.2%, -0.8%]	2
Improvements ✅ (secondary)	-5.0%	[-5.0%, -5.0%]	1
All ❌✅ (primary)	0.9%	[-1.2%, 2.9%]	4

Binary size

Results (primary -0.1%, secondary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.2%]	7
Regressions ❌ (secondary)	0.0%	[0.0%, 0.1%]	7
Improvements ✅ (primary)	-0.1%	[-0.5%, -0.0%]	64
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.0%]	50
All ❌✅ (primary)	-0.1%	[-0.5%, 0.2%]	71

Bootstrap: 691.904s -> 692.572s (0.10%)
Artifact size: 372.02 MiB -> 371.98 MiB (-0.01%)

ashivaram23 · 2025-06-20T06:01:48Z

Hmm that's unfortunate. It might help to avoid paying the cost of the extra clone and checks when it won't make a difference, like in the trivial otherwise targets when there's no wildcard. Maybe the otherwise could be handled in a separate pass after collecting variants to kill/add in a SmallVec, in case that helps somehow.

What's more concerning is runtime benchmarks being worse. There are only two, and I don't know how noisy they are, but 8% wall time increase for css-parse-fb is pretty bad. There are also some compile time benchmarks whose graphs show significant increases in wall time spent in LLVM. Does this just not play well with later optimizations?

rustbot · 2025-06-21T01:04:38Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

ashivaram23 · 2025-06-21T01:15:12Z

The last commit makes it so that there's an option in the MaybeUninitializedPlaces and MaybeInitializedPlaces builders for whether or not to update the otherwise block's lattice element, and it's set to only do so for MaybeInitializedPlaces in RemoveUninitDrops and ElaborateDrops. That should be enough to clean up some drops while hopefully being faster and not messing with other passes in ways that cause runtime slowdowns.

workingjubilee · 2025-06-21T01:42:24Z

the runtime benchmarks are unfortunately very noisy

fee1-dead · 2025-06-22T11:36:05Z

compiler/rustc_mir_dataflow/src/drop_flag_effects.rs

    move_data: &MoveData<'tcx>,
    enum_place: mir::Place<'tcx>,
    active_variant: VariantIdx,
    mut handle_inactive_variant: impl FnMut(MovePathIndex),
+    mut handle_active_variant: Option<impl FnMut(MovePathIndex)>,


I wonder if it will be better, perf-wise to make this not take an option but just use a no-op for callers that do not want to handle it.

That's definitely possible, since I think in that case on_all_children_bits could be monomorphized into an empty function. I made the change and it does reduce stage2 librustc_driver.so size a bit.

fee1-dead · 2025-06-22T11:37:27Z

this part of MIR is a little above my level of confidence for review, so

r? compiler

lcnr · 2025-06-23T12:01:54Z

r? wg-mir-opt

ashivaram23 · 2025-07-08T06:36:07Z

In the last commit I fixed the test file and incorporated the suggestions. I used a SmallVec since that's what SwitchTargets and other structs do, and I switched it to InactiveVariants for the double negative.

tmiasko · 2025-07-08T06:38:25Z

To me it seems like it should be called on the block's predecessor, but it's currently being passed the block itself.

Yes, it should have been called on the predecessor which contains the switch.

I haven't followed all the discussion, but if removing unused switch int handling in the backward direction makes anything easier that seems perfectly fine.

dianqk

Please squash your commits into one and update the PR description, remembering to use https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword.

dianqk · 2025-07-08T11:28:30Z

tests/mir-opt/otherwise_drops.rs

+
+// Ensures there are no drops for the wildcard match arm.
+
+// EMIT_MIR otherwise_drops.result_ok.ElaborateDrops.after.mir


Perhaps you want to add //@ compile-flags: -Cpanic=abort or // EMIT_MIR_FOR_EACH_PANIC_STRATEGY. I prefer the first one here.

dianqk · 2025-07-08T11:33:30Z

tests/mir-opt/otherwise_drops.rs

+fn result_ok(result: Result<String, ()>) -> Option<String> {
+    // CHECK-LABEL: fn result_ok(
+    // CHECK-NOT: drop
+    // CHECK: return


Suggested change

// CHECK: return

The return statement isn't the final line of this function.

dianqk · 2025-07-08T11:34:26Z

tests/mir-opt/otherwise_drops.rs

+fn main() {
+    result_ok(Err(()));
+}


Suggested change

fn main() {

result_ok(Err(()));

}

Removing this function makes file check easier.

dianqk · 2025-07-08T11:54:46Z

@bors2 try @rust-timer queue

rust-bors · 2025-07-08T11:54:50Z

⌛ Trying commit 881c62f with merge 237f435…

To cancel the try build, run the command @bors2 try cancel.

Apply effects to `otherwise` edge in dataflow analysis    This allows `ElaborateDrops` to remove drops when a `match` wildcard arm covers multiple no-Drop enum variants. It modifies dataflow analysis to update the `MaybeUninitializedPlaces` and `MaybeInitializedPlaces` data for a block reached through an `otherwise` edge. This appears to fix #142705, but I don't know for sure if it's actually correct (are there cases where this would be wrong and break things?). I also haven't tested that it actually improves compile times, or machine code output outside of the examples in the issue.

rust-bors · 2025-07-08T14:10:57Z

☀️ Try build successful (CI)
Build commit: 237f435 (237f435668e899ed46d943ff30ee6f097b6b03fb, parent: 45b80ac21a454d343833aad763ef604510c88375)

rust-timer · 2025-07-08T16:16:01Z

Finished benchmarking commit (237f435): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.7%, 0.7%]	1
Regressions ❌ (secondary)	0.6%	[0.2%, 0.9%]	13
Improvements ✅ (primary)	-0.4%	[-1.1%, -0.1%]	15
Improvements ✅ (secondary)	-0.3%	[-0.5%, -0.1%]	7
All ❌✅ (primary)	-0.3%	[-1.1%, 0.7%]	16

Max RSS (memory usage)

Results (primary 2.7%, secondary 3.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	4.8%	[3.5%, 6.2%]	2
Regressions ❌ (secondary)	3.2%	[3.2%, 3.2%]	1
Improvements ✅ (primary)	-1.7%	[-1.7%, -1.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.7%	[-1.7%, 6.2%]	3

Cycles

Results (primary 1.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.4%	[2.4%, 2.5%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.0%	[-2.0%, -2.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.0%	[-2.0%, 2.5%]	3

Binary size

Results (primary -0.1%, secondary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.0%, 1.4%]	10
Regressions ❌ (secondary)	0.0%	[0.0%, 0.1%]	4
Improvements ✅ (primary)	-0.1%	[-1.5%, -0.0%]	63
Improvements ✅ (secondary)	-0.1%	[-0.2%, -0.0%]	51
All ❌✅ (primary)	-0.1%	[-1.5%, 1.4%]	73

Bootstrap: 467.247s -> 463.296s (-0.85%)
Artifact size: 372.36 MiB -> 372.30 MiB (-0.02%)

ashivaram23 · 2025-07-08T23:10:00Z

Squashed and force pushed, the only changes since the last commit are addressing the test file review comments.

Regarding the perf run results, there are 8 cases this could affect:

MaybeInitializedPlaces in MIR type check liveness analysis
MaybeUninitializedPlaces in MIR borrow check
MaybeInitializedPlaces in SanityCheck
MaybeUninitializedPlaces in SanityCheck
MaybeInitializedPlaces in lint_tail_expr_drop_order
MaybeInitializedPlaces in RemoveUninitDrops
MaybeInitializedPlaces in ElaborateDrops
MaybeUninitializedPlaces in ElaborateDrops

The first perf run handled otherwise edges everywhere, the second did it only for 6 and 7, and the third did it for 6-8.

The first has the strongest results, with a lot of improvements and a few regressions. (Also a runtime test regression, which seems to follow a step function where it fluctuates between either exactly 0 or exactly 1.9% relative to the baseline over the past 30 days and went back to 1.9% here, not sure what that means)

The second still shows an overall improvement in compile times but applies to much fewer tests. It also has a different runtime regression with the same binary/step pattern.

The third has more regressions and fewer improvements, and an overall regression in compile times for all benchmarks (mostly due to the secondary benchmarks especially tt-muncher, it's a small overall improvement for the primary ones only). But there are no runtime test regressions and bootstrap times are four seconds faster.

Should I just enable the change for all 8 cases again? Or make it as limited as possible (number 7 alone is enough to fix the wildcard problem), or try other combinations?

dianqk · 2025-07-09T00:51:08Z

These can be subsequent PRs. Thanks!
@bors r+

bors · 2025-07-09T00:51:10Z

📌 Commit c7ef03a has been approved by dianqk

It is now in the queue for this repository.

bors · 2025-07-09T03:42:05Z

⌛ Testing commit c7ef03a with merge d350797...

bors · 2025-07-09T06:42:34Z

☀️ Test successful - checks-actions
Approved by: dianqk
Pushing d350797 to master...

github-actions · 2025-07-09T06:45:17Z

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 34097a3 (parent) -> d350797 (this PR)

Test differences

Show 12 test diffs

Stage 1

[mir-opt] tests/mir-opt/otherwise_drops.rs: [missing] -> pass (J1)

Stage 2

[mir-opt] tests/mir-opt/otherwise_drops.rs: [missing] -> pass (J0)

Additionally, 10 doctest diffs were found. These are ignored, as they are noisy.

Job group index

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard d350797b7e1a5b6952f5035cbad2a21613b32f6c --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

x86_64-apple-2: 4201.0s -> 5646.5s (34.4%)
aarch64-apple: 5870.2s -> 3930.0s (-33.1%)
x86_64-apple-1: 6297.3s -> 7782.0s (23.6%)
pr-check-2: 2186.0s -> 2662.0s (21.8%)
dist-apple-various: 5540.8s -> 6233.3s (12.5%)
dist-x86_64-apple: 10344.9s -> 9286.7s (-10.2%)
i686-gnu-2: 5414.8s -> 5928.7s (9.5%)
pr-check-1: 1492.8s -> 1621.1s (8.6%)
i686-gnu-1: 7288.5s -> 7894.3s (8.3%)
x86_64-gnu-debug: 5423.9s -> 5855.6s (8.0%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

rust-timer · 2025-07-09T07:54:23Z

Finished benchmarking commit (d350797): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.4%, 0.7%]	2
Regressions ❌ (secondary)	0.6%	[0.1%, 1.0%]	15
Improvements ✅ (primary)	-0.4%	[-1.3%, -0.1%]	18
Improvements ✅ (secondary)	-0.3%	[-0.5%, -0.0%]	10
All ❌✅ (primary)	-0.3%	[-1.3%, 0.7%]	20

Max RSS (memory usage)

Results (primary 2.7%, secondary 0.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.7%	[1.9%, 5.0%]	5
Regressions ❌ (secondary)	2.7%	[2.4%, 2.9%]	2
Improvements ✅ (primary)	-2.2%	[-2.2%, -2.2%]	1
Improvements ✅ (secondary)	-3.7%	[-3.7%, -3.7%]	1
All ❌✅ (primary)	2.7%	[-2.2%, 5.0%]	6

Cycles

Results (secondary -0.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.4%	[-3.4%, -3.4%]	1
All ❌✅ (primary)	-	-	0

Binary size

Results (primary -0.1%, secondary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.0%, 1.4%]	10
Regressions ❌ (secondary)	0.0%	[0.0%, 0.1%]	4
Improvements ✅ (primary)	-0.1%	[-1.5%, -0.0%]	70
Improvements ✅ (secondary)	-0.1%	[-0.2%, -0.0%]	52
All ❌✅ (primary)	-0.1%	[-1.5%, 1.4%]	80

Bootstrap: 467.127s -> 465.667s (-0.31%)
Artifact size: 372.35 MiB -> 372.26 MiB (-0.02%)

Kobzol · 2025-07-09T07:58:56Z

More improvements than regressions, and improvements are mostly in primary benchmarks, while regressions are in secondary benchmarks. This comment mentions a plan for possible future improvements.

@rustbot label: +perf-regression-triaged

rustbot assigned petrochenkov Jun 19, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 19, 2025

This comment has been minimized.

Sign in to view

rustbot assigned fee1-dead and unassigned petrochenkov Jun 19, 2025

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 19, 2025

This comment has been minimized.

Sign in to view

ashivaram23 force-pushed the drop_wildcard branch from 8d837ad to 10e1a9d Compare June 20, 2025 00:45

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jun 20, 2025

fee1-dead reviewed Jun 22, 2025

View reviewed changes

rustbot assigned lcnr and unassigned fee1-dead Jun 22, 2025

rustbot assigned dianqk and unassigned lcnr Jun 23, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 8, 2025

dianqk approved these changes Jul 8, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 8, 2025

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 8, 2025

ashivaram23 force-pushed the drop_wildcard branch from 881c62f to d9dd6ee Compare July 8, 2025 23:04

Apply effects to otherwise edge in dataflow analysis

c7ef03a

ashivaram23 force-pushed the drop_wildcard branch from d9dd6ee to c7ef03a Compare July 8, 2025 23:15

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 9, 2025

bors merged commit d350797 into rust-lang:master Jul 9, 2025
12 checks passed

rustbot added this to the 1.90.0 milestone Jul 9, 2025

rustbot added the perf-regression-triaged The performance regression has been triaged. label Jul 9, 2025


		// Ensures there are no drops for the wildcard match arm.

		// EMIT_MIR otherwise_drops.result_ok.ElaborateDrops.after.mir

Apply effects to otherwise edge in dataflow analysis #142707

Apply effects to otherwise edge in dataflow analysis #142707

Conversation

ashivaram23 commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jun 19, 2025

Uh oh!

This comment has been minimized.

petrochenkov commented Jun 19, 2025

Uh oh!

workingjubilee commented Jun 19, 2025

Uh oh!

This comment has been minimized.

rust-bors bot commented Jun 19, 2025

Uh oh!

rust-bors bot commented Jun 19, 2025

Uh oh!

This comment has been minimized.

rust-timer commented Jun 20, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

Uh oh!

ashivaram23 commented Jun 20, 2025

Uh oh!

rustbot commented Jun 21, 2025

Uh oh!

ashivaram23 commented Jun 21, 2025

Uh oh!

workingjubilee commented Jun 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fee1-dead commented Jun 22, 2025

Uh oh!

lcnr commented Jun 23, 2025

Uh oh!

ashivaram23 commented Jul 8, 2025

Uh oh!

tmiasko commented Jul 8, 2025

Uh oh!

dianqk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dianqk commented Jul 8, 2025

Uh oh!

This comment has been minimized.

rust-bors bot commented Jul 8, 2025

Uh oh!

rust-bors bot commented Jul 8, 2025

Uh oh!

This comment has been minimized.

rust-timer commented Jul 8, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

Uh oh!

ashivaram23 commented Jul 8, 2025

Uh oh!

dianqk commented Jul 9, 2025

Uh oh!

bors commented Jul 9, 2025

Uh oh!

bors commented Jul 9, 2025

Uh oh!

bors commented Jul 9, 2025

Uh oh!

Uh oh!

github-actions bot commented Jul 9, 2025

Test differences

Stage 1

Stage 2

Job duration changes

Uh oh!

rust-timer commented Jul 9, 2025

Apply effects to `otherwise` edge in dataflow analysis #142707

Apply effects to `otherwise` edge in dataflow analysis #142707

ashivaram23 commented Jun 19, 2025 •

edited

Loading